Skip to content

syntax: Simplify parsing of paths #43438

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Jul 28, 2017
Merged

syntax: Simplify parsing of paths #43438

merged 4 commits into from
Jul 28, 2017

Conversation

petrochenkov
Copy link
Contributor

Discern between Path and Path<> in AST (but not in HIR).
Give span to angle bracketed generic arguments (::<'a, T> in path::segment::<'a, T>).

This is a refactoring in preparation for https://internals.rust-lang.org/t/macro-path-uses-novel-syntax/5561/3, but it doesn't add anything to the grammar yet.

r? @jseyfried

petrochenkov
petrochenkov commented Jul 23, 2017
View reviewed changes
src/test/ui/span/macro-ty-params.rs
//~^ ERROR generic arguments in macro path
//~| ERROR generic arguments in macro path
//~| ERROR generic arguments in macro path
//~| ERROR generic arguments in macro path
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why resolve_macro_to_def is called so many times on macro paths, but it seems to be an orthogonal issue (and maybe a performance concern).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, it may be undetermined a few times and then resolved later. So the generic check need to be done only when resolution is successful.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense to do this check in fn finalize_current_module_macro_resolutions in librustc_resolve/macros.rs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the necessary information is already lost in finalize_current_module_macro_resolutions, so I kept it in resolve_macro_to_def.

@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 24, 2017
@jseyfried
Copy link
Contributor

Did a first pass-through review, LGTM modulo comment.

I'd like to do a second pass over the parser changes tomorrow, otherwise r=me.

@petrochenkov
Copy link
Contributor Author

I'd like to do a second pass over the parser changes tomorrow

The idea is that parse_path_segments_without_colons/parse_path_segments_with_colons/parse_path_segments_without_types are merged into a single parse_path_segments calling parse_path_segment in a loop, which in its turn does all the interesting job. Method calls and field accesses now also use this parse_path_segment instead of custom code.
This is mostly a rewrite, so reading the new code may be more convenient than the diff.

@bors
Copy link
Collaborator

bors commented Jul 26, 2017

☔ The latest upstream changes (presumably #43487) made this pull request unmergeable. Please resolve the merge conflicts.

@jseyfried
Copy link
Contributor

Looks great! r=me

@petrochenkov
Copy link
Contributor Author

@bors r=jseyfried

@bors
Copy link
Collaborator

bors commented Jul 27, 2017

📌 Commit 1e8a7f6 has been approved by jseyfried

@bors
Copy link
Collaborator

bors commented Jul 27, 2017

⌛ Testing commit 1e8a7f6 with merge 8a78a12...

bors added a commit that referenced this pull request Jul 27, 2017
@bors
Auto merge of #43438 - petrochenkov:path, r=jseyfried
8a78a12 
syntax: Simplify parsing of paths

Discern between `Path` and `Path<>` in AST (but not in HIR).
Give span to angle bracketed generic arguments (`::<'a, T>` in `path::segment::<'a, T>`).

This is a refactoring in preparation for https://internals.rust-lang.org/t/macro-path-uses-novel-syntax/5561/3, but it doesn't add anything to the grammar yet.

r? @jseyfried
@bors
Copy link
Collaborator

bors commented Jul 28, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: jseyfried
Pushing 8a78a12 to master...

@bors bors merged commit 1e8a7f6 into rust-lang:master Jul 28, 2017
@petrochenkov petrochenkov deleted the path branch August 26, 2017 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@jseyfried jseyfried jseyfried left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@jseyfried jseyfried

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@petrochenkov @jseyfried @bors @carols10cents